Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

feat(core): Pass name & attributes to tracesSampler #10426

Merged
merged 2 commits into from
Jan 31, 2024

Conversation

mydea
Copy link
Member

@mydea mydea commented Jan 30, 2024

Updates tracesSampler to receive name and attributes instead of relying on transactionContext being passed.

@gajus
Copy link
Contributor

gajus commented Jan 30, 2024

@mydea I will check this in a second. Looks promising. Thank you.

However, I suspect, this will be only partial solution to the problem I am seeking to solve.

The greater problem is that tracesSampler is called before either of these hooks are called:

getNodeAutoInstrumentations({
  '@opentelemetry/instrumentation-fastify': {
    requestHook: () => {},
  },
  '@opentelemetry/instrumentation-http': {
    requestHook: () => {},
  },
}),

... and the data I need to make the sampling decision can only be obtained by referencing request.session, which is instantiated a lot later.

So I am struggling to understand what's the correct way to determine sampling when the data for sampling decision only becomes during the transaction and not at the start of the transaction.

Copy link
Contributor

github-actions bot commented Jan 30, 2024

size-limit report 📦

Path Size
@sentry/browser (incl. Tracing, Replay, Feedback) - Webpack (gzipped) 78.13 KB (+0.03% 🔺)
@sentry/browser (incl. Tracing, Replay) - Webpack (gzipped) 69.37 KB (+0.04% 🔺)
@sentry/browser (incl. Tracing, Replay with Canvas) - Webpack (gzipped) 73.24 KB (+0.05% 🔺)
@sentry/browser (incl. Tracing, Replay) - Webpack with treeshaking flags (gzipped) 62.98 KB (+0.04% 🔺)
@sentry/browser (incl. Tracing) - Webpack (gzipped) 33.38 KB (+0.07% 🔺)
@sentry/browser (incl. browserTracingIntegration) - Webpack (gzipped) 33.25 KB (+0.07% 🔺)
@sentry/browser (incl. Feedback) - Webpack (gzipped) 31.33 KB (0%)
@sentry/browser (incl. sendFeedback) - Webpack (gzipped) 31.34 KB (0%)
@sentry/browser - Webpack (gzipped) 22.6 KB (0%)
@sentry/browser (incl. Tracing, Replay, Feedback) - ES6 CDN Bundle (gzipped) 76.1 KB (+0.04% 🔺)
@sentry/browser (incl. Tracing, Replay) - ES6 CDN Bundle (gzipped) 67.65 KB (+0.04% 🔺)
@sentry/browser (incl. Tracing) - ES6 CDN Bundle (gzipped) 33.49 KB (+0.09% 🔺)
@sentry/browser - ES6 CDN Bundle (gzipped) 24.66 KB (0%)
@sentry/browser (incl. Tracing, Replay) - ES6 CDN Bundle (minified & uncompressed) 213.48 KB (+0.05% 🔺)
@sentry/browser (incl. Tracing) - ES6 CDN Bundle (minified & uncompressed) 101.49 KB (+0.1% 🔺)
@sentry/browser - ES6 CDN Bundle (minified & uncompressed) 74 KB (0%)
@sentry/browser (incl. Tracing) - ES5 CDN Bundle (gzipped) 36.62 KB (+0.06% 🔺)
@sentry/react (incl. Tracing, Replay) - Webpack (gzipped) 69.73 KB (+0.02% 🔺)
@sentry/react - Webpack (gzipped) 22.63 KB (0%)
@sentry/nextjs Client (incl. Tracing, Replay) - Webpack (gzipped) 86.5 KB (+0.03% 🔺)
@sentry/nextjs Client - Webpack (gzipped) 50.8 KB (+0.05% 🔺)
@sentry-internal/feedback - Webpack (gzipped) 17.21 KB (0%)

@mydea
Copy link
Member Author

mydea commented Jan 30, 2024

The greater problem is that tracesSampler is called before either of these hooks are called:

Hmm, that's of course a problem - what span is the sampling based off on then, I wonder? sampling runs for the "root" span, so if that does not contain the info you care about, you're out of luck, I fear, with the current model... 🤔

@gajus
Copy link
Contributor

gajus commented Jan 30, 2024

Hmm, that's of course a problem - what span is the sampling based off on then, I wonder? sampling runs for the "root" span, so if that does not contain the info you care about, you're out of luck, I fear, with the current model... 🤔

Perhaps I am missing something obvious, but why can't sampling decision be made at the end of the request based on attributes contained within spans?

@mydea mydea force-pushed the fn/sampling-context-attributes branch from 0f8354b to 3cec51d Compare January 30, 2024 16:24
@mydea
Copy link
Member Author

mydea commented Jan 30, 2024

Hmm, that's of course a problem - what span is the sampling based off on then, I wonder? sampling runs for the "root" span, so if that does not contain the info you care about, you're out of luck, I fear, with the current model... 🤔

Perhaps I am missing something obvious, but why can't sampling decision be made at the end of the request based on attributes contained within spans?

That's sadly a fundamental change to how sampling works with Sentry - we always do head-based sampling, as of now. This limitation is unlikely to be changed in the near future, although we are always exploring ways to change this potentially.

One of the reasons that this becomes tricky is that we want to ensure to inherit the sampling decision for child spans. So if we'd only sample a parent span when it is finished, we could not know and cascade this for child spans that may be finished before that 🤔

@gajus
Copy link
Contributor

gajus commented Jan 30, 2024

One of the reasons that this becomes tricky is that we want to ensure to inherit the sampling decision for child spans. So if we'd only sample a parent span when it is finished, we could not know and cascade this for child spans that may be finished before that 🤔

I realize this is not a quick fix, but could this be achieved by allowing clients to deploy a service that buffers traces before they are sent to Sentry, and allow to decide whether to keep/reject traces inside of that buffer once information is available about the entire trace?

Presumably, I should be able to cook this up myself as long as Sentry allows to configure custom destination.

@gajus
Copy link
Contributor

gajus commented Jan 30, 2024

@mydea Related – how do I know inside of request if the request was sampled or not?

I need to generate and inject sentry-trace meta attribute.

<meta content="${sentryTrace}" name="sentry-trace" />

I currently do this:

import { context, trace } from '@opentelemetry/api';

const activeContext = context.active();

const activeSpan = trace.getSpan(activeContext);

if (!activeSpan) {
  return {
    sentryTrace: null,
    spanId: null,
    traceId: null,
  };
}

const { spanId, traceId } = activeSpan.spanContext();

const sentryTrace = `${traceId}-${spanId}-0`;

but need to figure out how to adjust -0 to -1 when the request is sampled.

MIGRATION.md Outdated Show resolved Hide resolved
mydea and others added 2 commits January 31, 2024 13:05
Co-authored-by: Luca Forstner <luca.forstner@sentry.io>
@mydea mydea force-pushed the fn/sampling-context-attributes branch from 11e6edf to e352d7d Compare January 31, 2024 12:06
@mydea mydea merged commit 369faf4 into develop Jan 31, 2024
98 checks passed
@mydea mydea deleted the fn/sampling-context-attributes branch January 31, 2024 13:11
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants